Repository navigation
Conversation
…ediate is queued ref() after unref() took an event loop ref for an Immediate that had left the immediate queue: inside its own callback, or after the idle loop dropped it. The ref came back only when the wrapper was collected, so the process stayed alive while the handle was reachable. An Immediate on the immediate queue now reports _destroyed === true inside its callback, as in Node.js: only the timer heap sets in_callback.
|
Updated 8:10 AM PT - Nov 11th, 2026
✅ @robobun, your commit 9fac4f208d1499742e1d3e0cd707b7434f7a8638 passed in 🧪 To try this PR locally: bunx bun-pr 48576That installs a local version of the PR into your bun-48576 --bun |
|
Status: ready for review. One decision is open: How I reproduced it: With this change the same script exits in about 20 ms. CI, build 140143: 184 of 188 jobs passed, on Linux (glibc, musl, ASAN), Windows and macOS arm64. No lane reported a failure in |
|
No actionable comments were generated in the recent review. WalkthroughTimer internals now bracket callback execution with ChangesImmediate timer lifecycle
Possibly related PRs
Suggested reviewers: Merge Risk:
|
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I traced every transition of this_value for a real setImmediate (init pins it after taking the ref; run_immediate_task on both the dropped and run branches, cancel_pending_immediate, and cancel all downgrade it) and found no path that leaves a stale strong pin, so the is_queued_immediate gate cannot re-take the loop ref on a cleared or already-run Immediate; the in_callback bracket moved into fire_catching has no early return between set and clear and update_flags re-reads the Cell, so a re-entrant cancel() during a Timeout callback is still not clobbered. Not run here: the new subprocess tests, the USE_SYSTEM_BUN=1 failure check, and the clause-removal check the description reports.
Extended reasoning...
The change is confined to the timer keep-alive logic in src/runtime/timer/timer_object_internals.rs (a new pin-based gate in ref() for real Immediates, a debug assertion, and moving the in_callback set/clear from run() to fire_catching), a one-line doc update in src/event_loop/EventLoopTimer.rs, and new node-timers tests; it touches no security-sensitive surface. Two confirmed findings are posted inline (the beforeExit divergence from Node and the faked-vs-real Immediate _destroyed inconsistency), which is why this is a defer rather than an approve; no CODEOWNERS entry covers the changed files.
Problem
ref()afterunref()inside an Immediate's own callback takes an event loop ref that stays while the handle is reachable:const a = setImmediate(() => a.ref()); setImmediate(() => {}); a.unref();never exits. Node exits. Regressed in v1.2.10 (Fix setImmediate slowness #18889). No user report.do_ref(src/runtime/timer/timer_object_internals.rs:999) takes the ref when!get_destroyed(). That is true inside the callback, and for an unref'ed Immediate that bun removed from the queue unrun.Fix
ref()takes the ref for asetImmediateonly while the wrapper pin says it is queued. A debug assertion catches a call site that skips the gate.in_callback. Inside its callback an Immediate now reports_destroyed === trueandhasRef() === false, as in Node.test/js/node/timers/node-timers.test.ts(10 tests fail on main). Alsotest/js/web/timers/,fake-timers.test.ts, 56 vendoredtest-timers-*.js.Background
setImmediateruns from the immediate queue. Timeouts and faked Immediates fire from the timer heap.this_value) is a strong GC handle until the Immediate leaves the queue.in_callbackkeeps_destroyedfalse during a callback, as Node does for Timeouts.get_destroyed()arm alone: the removed Immediate still hangs.Notes
Where it came from. A generated program that mixes
ref()andunref()on Immediates did not end on bun and ended on Node. Review of #30069 had named the same gap (unref(); ref()inside asetImmediatecallback) and left it for a follow-up that aligns_destroyedfor Immediates. The two repro shapes exit on bun 1.2.9 and hang on 1.2.10.How long the ref stays.
deinit()gives the ref back when the wrapper is collected. So the process stays alive for good while the handle is reachable: the script above as an ES module or withbun -ewas still running after 60 s on main. As a CommonJS file, where nothing holdsaafter the module body, it exited after about 11 s. In a Worker the worker had not exited after 45 s on main, and it exits here.Measurements. Two release builds from one toolchain: main 89452d2 and this change. Instruction counts come from gdb (
nextiinside the function, orstepiuntil it returns), two equal runs each.__bun_run_immediate_task, bytes__bun_run_immediate_task, instructions per ref'edsetImmediate(exclusive)in_callbackstores in__bun_run_immediate_taskfire_catching, bytesfire_catching, instructions persetTimeout(fn, 0)callbackfire_catching, instructions persetIntervaltickTimeout.ref()afterunref(), instructions (inclusive)Immediate.ref()afterunref(), still queuedref()when already ref'ed, Timeout and Immediate_destroyedgetters, instructionsBun__Timer__setImmediate, instructions (exclusive / inclusive, second call)Bun__Timer__setTimeout, instructions (exclusive)TimerObjectInternals::init, instructions (setImmediate/setTimeout)initandset_enable_keeping_event_loop_alive, bytesrefhost functions plus the new out-of-line function, bytessetImmediate(mimallocmalloc_normal_countslope)epoll_pwait2catchpoint hits for 20000 and 40000 chainedsetImmediatesize build/release/bunTimerObjectInternalsstays 40 bytes.TimerFlagsstays at 32 of 32 bits. The diff ofEventLoopTimer.rsis one comment line.Two earlier shapes of this change were dropped for cost. A pin of the wrapper before the kind branch in
init()added 3 instructions persetImmediate()and 2 persetTimeout(). Soinit()is unchanged, and the assertion accepts the empty slot thatinit()has when it takes the ref. With the gate inline, LLVM stopped inliningdo_refinto the two host functions, which added 18 instructions to everyref(). So the path afterunref()is one out-of-line function.A smaller shape that was not taken. Mark the removed Immediate cleared in
run_immediate_taskand keep the!get_destroyed()test indo_ref. That needs no new function. It makes_destroyedtrue for an Immediate that bun removed unrun, where Node prints false, and it adds a store to the path of every cleared Immediate.The assertion. It reads the same pin as the gate, so it catches a future call site that enables the ref without the gate. It does not catch a pin that no longer matches the queue.
Probes against Node v26.3.0. 26 scripts that mix
ref(),unref(),clearImmediate,beforeExit,uncaughtException, a Worker and timers. On main 17 print something else than Node, and in 11 the loop is still held when the guard timer of the script fires or at the 6 s limit. With this change 8 differ and none of the 26 hangs. The 8 are:ref()on an Immediate that bun removed unrun, 4 scripts. One of them matched Node on main: it calledunref()again at once.ref()involved, 2 scripts.Timeout.hasRef()after the timer fired or was cleared, 2 scripts. Node keeps the lastref()/unref()state. timers: Timeout.hasRef() reports the sticky ref flag after clearTimeout #34082 changed that and closed unmerged.Each clause has a failing test. On debug builds with one clause removed:
beforeExittest fails. The child aborts withpanic: an Immediate that left the queue took an event loop ref.in_callbackset from the immediate queue again: the 8 script rows and "is true during the callback of an immediate" fail on the_destroyedvalue. No row hangs, so thein_callbackmove can be split out if the_destroyedchange is not wanted.!fake(): "ref() inside the callback of a faked Immediate takes the ref" fails. That test passes on main too. It pins the timer heap behaviour of a faked Immediate.Suites. Release builds, main against this change,
node-timers.test.tsplus all oftest/js/web/timers/plusfake-timers.test.ts: 610 pass and 10 fail on main, 620 pass and 0 fail here. Debug (ASAN) build:node-timers.test.ts44 of 44,setTimeout.test.js,setImmediate.test.js,fake-timers.test.ts497 of 497, 56 vendoredtest-timers-*.js,test-process-ref-unref.js. On the debug buildtimer-gc-roots.test.tshas 2 tests andsetInterval.test.jshas 1 test that exceed their time limits. Their fixtures finish with the right output without the limit, they do not usesetImmediate, and both files pass on the two release builds.The tests spawn with
timeout.bun testdoes not kill the child of a concurrent test that times out, so a failing run without it leaves hung children.Still unlike Node after this change.
setImmediate(a); setImmediate(b).unref()runs onlya, where Node runs both in one pass. For the same reasontimers/promisessetImmediate(v, { ref: false })behind a plainsetImmediatedoes not settle. When nothing else holds the loop, Node keeps the Immediate queued and runs it ifbeforeExitrevives the loop.ref()are unchanged: abeforeExitlistener that queues an unref'ed Immediate, and a top-levelawaitofsetImmediate(v, { ref: false }).Timeout.hasRef()after fire or clear (above).jest.useFakeTimers({ toFake: ["setImmediate"] })) keeps_destroyed === falseandhasRef() === trueinside its callback.The first two need the drop rule in
run_immediate_taskand loop liveness (is_event_loop_alive) to change. That is a separate change.Self-review. Four concerns. (1) The spawned children had no
timeout, so a failing run left hung children: added. (2) ThebeforeExittest printed nothing afterref(): it now printshasRef()afterref()and states what Node prints. (3) The comment above the gate cited Node's Immediate rule under a sentence about_destroyed, and the source did not say thatref()on a removed Immediate differs from Node: the comment now covers Timeouts only and two one-line comments cover Immediates. (4) "for good" was too strong: see "How long the ref stays". Declined: an in-process test of the removed Immediate. It passes only if nothing else holds the loop of the test runner, which the test does not control.Merge order. #48149 and #44806 also edit
fire_catching.Windows. Not run. There the Immediate loop ref is a started
uv_idlehandle (increment_immediate_ref), which keepsuv_runfrom sleeping in poll. So by reading, the leaked ref on main also keeps the loop busy there.Downsides
Immediate._destroyedinside its own callback changes fromfalsetotrue. This reverses an assertion from node:timers fixes #16855.ref()on an unref'ed Immediate that bun removed unrun now does nothing:hasRef()staysfalse. Node runs it. Before, bun hung.ref()afterunref(): 13 more instructions for a Timeout (71 to 84), 11 for a queued Immediate.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/timers/node-timers.test.ts